Skip to content

Conversation

Milo123459
Copy link
Contributor

Fixes #99135

r? @jyn514

@rustbot rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Jul 20, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 20, 2022
@Milo123459 Milo123459 requested a review from jyn514 July 23, 2022 19:25
@jyn514
Copy link
Member

jyn514 commented Jul 29, 2022

I think this will only run on PRs and not the main bors merge? that seems not great, it means you can hit semantic conflicts (although I guess those are rare in practice).

@pietroalbini how can I tell if this will run the new tests on a full merge or not?

@jyn514
Copy link
Member

jyn514 commented Jul 29, 2022

err pietro is out - will just ask someone random I guess.

r? rust-lang/infra-ci

@rust-highfive rust-highfive assigned pietroalbini and unassigned jyn514 Jul 29, 2022
@jyn514
Copy link
Member

jyn514 commented Jul 29, 2022

highfive pls

r? rust-lang/infra-ci

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jul 30, 2022

The new builder was only added to the pr: stanza in the configuration, so it won't run on a full build. This is something we should definitely fix.

If we're going to run the builder in PR CI, I would probably recommend we drop fulldeps tests and the other bits that are still causing two full compiler builds for it -- IIRC, @jyn514 you were looking into that on Zulip at some point for other reasons, though I forget the details.

@jyn514
Copy link
Member

jyn514 commented Jul 30, 2022

yeah, that was #99141 - it ended up not having any time savings at all, which I think means stage 2 was still being compiled. Haven't had time to investigate - not sure it makes sense to block this change because of that.

@jyn514
Copy link
Member

jyn514 commented Jul 30, 2022

anyway, @Milo123459 I'm happy to approve if you change this to run in the full test suite as well as in PR checks

@jyn514
Copy link
Member

jyn514 commented Jul 30, 2022

@bors r+ rollup=iffy

@bors
Copy link
Collaborator

bors commented Jul 30, 2022

📌 Commit 0f121d7 has been approved by jyn514

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 30, 2022
@Mark-Simulacrum
Copy link
Member

I'm not sure it makes sense to run this in PR checks. There's not really a time benefit that I can see, and those builders will add up pretty quickly given how many PRs we have. Hopefully, defects here are pretty rare, so it doesn't seem like a good trade-off to me to spend that CI budget on this builder.

Is there some evidence that breaking stage 1 is common enough that we want the early warning at PR time rather than bors time?

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 30, 2022
@jyn514
Copy link
Member

jyn514 commented Jul 30, 2022

ah, fair enough - I still had it in my mind that we were doing this instead of stage2 builds, which isn't the case. Yes, I agree it's better not to run in PR checks.

@Milo123459
Copy link
Contributor Author

Alright, stage1 tests are no longer run in PRs, only in the full test-suite.

@jyn514
Copy link
Member

jyn514 commented Jul 30, 2022

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 30, 2022

📌 Commit 1f7b655 has been approved by jyn514

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 30, 2022
@bors
Copy link
Collaborator

bors commented Jul 31, 2022

⌛ Testing commit 1f7b655 with merge 482153b...

@bors
Copy link
Collaborator

bors commented Jul 31, 2022

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 482153b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 31, 2022
@bors bors merged commit 482153b into rust-lang:master Jul 31, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 31, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (482153b): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)
1.6% 1.6% 1
Regressions 😿
(secondary)
2.2% 2.2% 1
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) 1.6% 1.6% 1

Cycles

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)
2.0% 2.0% 1
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
-3.2% -3.6% 2
Improvements 🎉
(secondary)
-7.0% -11.7% 5
All 😿🎉 (primary) -1.4% -3.6% 3

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run x test --stage 1 in CI
9 participants